uws: make stream-buffer conversion a one-shot ownership transfer by robobun · Pull Request #31983 · oven-sh/bun

robobun · 2026-06-08T09:46:46Z

Fixes #31971

Problem

us_socket_stream_buffer_t::to_stream_buffer(&self) rebuilt an owning Vec<u8> with Vec::from_raw_parts and left list_ptr/list_len/list_cap in place. Two calls, or one call followed by us_socket_free_stream_buffer, gave two owners of one allocation. Safe Rust could reach it: free(): double free detected in tcache 2, or under Miri constructing invalid value of type &mut [u8]: encountered a dangling reference (use-after-free).
The cause is the safe &self signature in src/uws_sys/us_socket_t.rs. The one caller, us_socket_buffered_js_write in src/runtime/socket/uws_jsc.rs, avoided the hazard by call-site discipline only.

Fix

to_stream_buffer(&self) becomes take_stream_buffer(&mut self). It rebuilds the Vec, then nulls the four raw-part fields. A second take returns an empty buffer. total_bytes_written is socket state and survives.
destroy and update route through the take. destroy is idempotent. update drops a buffer it still owns before it overwrites the parts (that was a silent leak).
The caller behaves the same: it coerces JS input first, takes, writes, then hands ownership back with update. Only the signature enforces the contract now.
Verified: cargo miri test -p bun_uws_sys (6 new tests, 8 total) and test/internal/source-lints/uws-stream-buffer-ownership.test.ts. Also bun bd test on the node:http backpressure, nested-cork, ondata-reregister and main suites.

Background

us_socket_stream_buffer_t is a #[repr(C)] struct that C++ (JSNodeHTTPServerSocket) embeds by value. It holds the raw parts of a Vec<u8> so C++ can read them. Only Rust rebuilds the Vec, in take_stream_buffer.
bun_uws_sys now runs in the cargo miri test CI job (MIRI_CRATES in scripts/rust-miri.ts). Miri interprets the tests and reports a double free or a leak as a hard error.
The source lint pins the &mut self signature and the single Vec::from_raw_parts site. It lives in test/internal/source-lints/, which runs on GitHub Actions without a build.

Notes

Mutation checks under Miri: remove the four field resets in take_stream_buffer and destroy_after_take_is_a_noop fails with the dangling-reference error above. Remove the drop(self.take_stream_buffer()) in update and update_drops_the_previously_owned_buffer fails with memory leaked.

Rebase onto main (ae3c3ad) resolved three conflicts. .github/workflows/miri.yml no longer exists: main folded the miri job into rust-lints.yml, whose path filter already covers src/**/*.rs, so this PR only adds the crate to MIRI_CRATES. destroy on main had changed to &mut self and already nulled list_ptr/list_cap; this PR keeps that signature and routes it through the take, which also clears list_len and cursor. The Default impl the tests used was deleted on main as dead code (#39795), so the test module has a private empty() constructor that mirrors the C++ zero-initialization.

The unsound path was never reachable from JS because the one caller ordered around it. The three crashes the issue-finder bot linked (#31467, #27929, #31004) are on Bun.serve and uWS teardown paths that do not use this buffer, so this PR does not close them.

[human-review] gate passed · iteration 6 · 4 files touched

fails on main (without fix)

ASAN without fix: 2 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/internal/source-lints/uws-stream-buffer-ownership.test.ts
bun test v1.4.3 (f42e98025)

test/internal/source-lints/uws-stream-buffer-ownership.test.ts:
23 | const source = readFileSync(path.join(root, "src/uws_sys/us_socket_t.rs"), "utf8").replace(/\s+/g, " ");
24 | 
25 | test("stream-buffer conversion is a one-shot take (&mut self), not a safe &self copy (#31971)", () => {
26 |   // The conversion must require exclusive access so it can null the raw
27 |   // parts whose ownership it transfers out.
28 |   expect(source).toMatch(/\bpub\s+fn\s+take_stream_buffer\s*\(\s*&\s*mut\s+self\s*\)\s*->\s*StreamBuffer\b/);
                      ^
error: expect(received).toMatch(expected)

Expected substring or pattern: /\bpub\s+fn\s+take_stream_buffer\s*\(\s*&\s*mut\s+self\s*\)\s*->\s*StreamBuffer\b/
Received: "use core::ffi::{c_char, c_int, c_uint, c_void}; use core::ptr::{self, NonNull}; use bun_core::Fd; use crate::{LIBUS_SOCKET_DESCRIPTOR, SocketGroup, SocketKind, SslCtx, us_bun_verify_error_t}; bun_core::declare_scope!(uws, visible); cons
... (truncated)

release without fix: 2 FAILED
bun test v1.4.3-canary.1 (f42e98025)

test/internal/source-lints/uws-stream-buffer-ownership.test.ts:
23 | const source = readFileSync(path.join(root, "src/uws_sys/us_socket_t.rs"), "utf8").replace(/\s+/g, " ");
24 | 
25 | test("stream-buffer conversion is a one-shot take (&mut self), not a safe &self copy (#31971)", () => {
26 |   // The conversion must require exclusive access so it can null the raw
27 |   // parts whose ownership it transfers out.
28 |   expect(source).toMatch(/\bpub\s+fn\s+take_stream_buffer\s*\(\s*&\s*mut\s+self\s*\)\s*->\s*StreamBuffer\b/);
                      ^
error: expect(received).toMatch(expected)

Expected substring or pattern: /\bpub\s+fn\s+take_stream_buffer\s*\(\s*&\s*mut\s+self\s*\)\s*->\s*StreamBuffer\b/
Received: "use core::ffi::{c_char, c_int, c_uint, c_void}; use core::ptr::{self, NonNull}; use bun_core::Fd; use crate::{LIBUS_SOCKET_DESCRIPTOR, SocketGroup, SocketKind, SslCtx, us_bun_verify_error_t}; bun_core::declare_scope!(uws, visible); const MAX_I32: usize = i32::MAX as usize; // Rust bindings for `us_socket_t`. // // TLS is per-socket (`s->ssl != NULL` in C); there is no `int ssl` selector. // Dispatch is by `kind()` — 
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/internal/source-lints/uws-stream-buffer-ownership.test.ts
bun test v1.4.3 (f42e98025)

test/internal/source-lints/uws-stream-buffer-ownership.test.ts:
(pass) stream-buffer conversion is a one-shot take (&mut self), not a safe &self copy (#31971) [12.02ms]
(pass) take_stream_buffer is the only place that rebuilds the Vec from raw parts (#31971) [2.62ms]

 2 pass
 0 fail
 3 expect() calls
Ran 2 tests across 1 file. [2.16s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 688ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/5] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 242 extern-C blocks audited
[1/5] cargo bun_runtime → libbun_runtime.a
�[1m�[92m   Compiling�[0m bun_uws_sys v0.0.0 (/workspace/bun/src/uws_sys)
�[1m�[92m   Compiling�[0m bun_io v0.0.0 (/workspace/bun/src/io)
�[1m�[92m   Compiling�[0m bun_uws v0.0.0 (/workspace/bun/src/uws)
�[1m�[92m   Compiling�[0m bun_crash_handler v0.0.0 (/workspace/bun/src/crash_handler)
�[1m�[92m   Compiling�[0m bun_event_loop v0.0.0 (/workspace/bun/src/event_loop)
�[1m�[92m   Compiling�[0m bun_sourcemap v0.0.0 (/workspace/bun/src/sourcemap)
�[1m�[92m   Compiling�[0m bun_css v0.0.0 (/workspace/bun/src/css)
�[1m�[92m   Compiling�[0m bun_http v0.0.0 (/workspace/bun/src/http)
�[1m�[92m   Compiling�[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser)
�[1m�[92m   Compiling�[0m bun_spawn v0.0.0 (/workspace/bun/src/spawn)
�[1m�[92m   Compiling�[0m bun_js_printer v0.0.0 (/workspace/bun/src/js_printer)
�[1m�[92m   Compiling�[
... (truncated)

diff hotspot

scripts/rust-miri.ts                               |   1 +
 src/runtime/socket/uws_jsc.rs                      |  14 +-
 src/uws_sys/us_socket_t.rs                         | 145 +++++++++++++++++----
 .../uws-stream-buffer-ownership.test.ts            |  37 ++++++
 4 files changed, 162 insertions(+), 35 deletions(-)

gate history · 1 passed · 0 rejected · iteration 6

evidence per changed file

file                                                      reads  edits  tests
scripts/rust-miri.ts                                          1      2     12
src/runtime/socket/uws_jsc.rs                                 1      3     13
src/uws_sys/us_socket_t.rs                                    4      8     18
…ternal/source-lints/uws-stream-buffer-ownership.test.ts      1      3      7

root cause · written by the author bot

The safe method us_socket_stream_buffer_t::to_stream_buffer(&self) rebuilt a Vec with Vec::from_raw_parts from pointer, length, and capacity fields it never cleared, so calling it twice produced two owning vectors over the same allocation and a double free through safe Rust. The fix replaces the &self conversion with a consuming take that moves the raw parts out and resets them to an empty state, making the ownership transfer one-shot, and updates the caller in uws_jsc.rs accordingly. A source lint in test/internal/source-lints/ guards against reintroducing an owning conversio…

coderabbitai · 2026-06-08T09:46:54Z

Warning

Review limit reached

Run on-demand review

_{On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file.}

Or wait 13 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: e927f4bc-7228-444e-a333-4a5e0b6f433a

📥 Commits

Reviewing files that changed from the base of the PR and between b52d3e5 and 33c5eb0.

📒 Files selected for processing (4)

scripts/rust-miri.ts
src/runtime/socket/uws_jsc.rs
src/uws_sys/us_socket_t.rs
test/internal/source-lints/uws-stream-buffer-ownership.test.ts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-06-08T09:46:58Z

^{Updated 8:05 PM PT - Sep 7th, 2026}

❌ @robobun, your commit 33c5eb0 has 2 failures in Build #112419 (All Failures):

test/js/web/timers/setInterval.test.js - code 1 on 🐧 13 x64-asan
test/js/node/test/parallel/test-crypto-dh-leak.js - code 1 on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 31983

That installs a local version of the PR into your bun-31983 executable, so you can run:

bun-31983 --bun

github-actions · 2026-06-08T09:51:25Z

Found 3 issues this PR may fix:

Segfault in uWS::HttpContext<false>::onClose during socket teardown (Bun 1.3.14, Linux, standalone executable) #31467 - Segfault in uWS::HttpContext::onClose during socket teardown on a long-running HTTP server. Heap corruption from the double-free could cause crashes during socket close.
Heap corruption (malloc free list) during long-running server with sharp + mongodb #27929 - Explicitly reports "malloc: double free for ptr" and heap corruption on a long-running Bun.serve() server — exact symptom of this bug.
Recurring segfault after POST to a Hono route mounting @hono/mcp StreamableHTTPTransport (Bun 1.3.13 and 1.3.14) #31004 - Recurring segfault after HTTP POST response on Hono + Bun.serve at null-pointer-offset addresses, consistent with use-after-free in write buffer.

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #31467
Fixes #27929
Fixes #31004

🤖 Generated with Claude Code

robobun · 2026-06-08T09:52:22Z

Not adding the suggested "Fixes" lines: this PR should not auto-close those issues.

The unsound signature fixed here was a latent API hazard, not a live runtime bug. The only caller of the conversion, us_socket_buffered_js_write (the node:http server socket write path), already coerced JS values before taking the buffer and handed ownership back via update() before returning, with no JS executing in between, so the double free was not reachable from any shipped code path. This PR changes no runtime behavior; it makes the one-shot contract structural.

The three issues also sit on different paths: #31467 crashes in uWS C++ HttpContext::onClose teardown, and #27929 / #31004 are Bun.serve() fetch-handler servers (#27929 additionally loads the sharp native addon), none of which go through JSNodeHTTPServerSocket buffered writes. Closing them on merge would leave real bugs marked fixed.

claude

I didn't find any issues — the ownership-transfer rework looks correct and well-tested — but since this changes unsafe raw-pointer ownership semantics on the node:http server write path and adds a crate to the Miri lane, I'd like a human to confirm before merging.

Extended reasoning...

Overview

This PR fixes a soundness hole in us_socket_stream_buffer_t: the old to_stream_buffer(&self) rebuilt an owning Vec<u8> via Vec::from_raw_parts without clearing the stored raw parts, so two safe calls could produce two owners of one allocation (double free). The fix renames it to take_stream_buffer(&mut self) and nulls list_ptr/list_len/list_cap/cursor after the rebuild, routes destroy and update through it (making destroy idempotent and update leak-free), updates the sole caller in src/runtime/socket/uws_jsc.rs, adds six Rust unit tests, wires bun_uws_sys into MIRI_CRATES + the workflow path filter, and adds a source-text lint under test/internal/.

Security risks

None introduced. The change strictly tightens memory safety: it removes a double-free reachable from safe Rust and a silent leak in update. No new attack surface, no auth/crypto/permissions involved.

Level of scrutiny

High. The code under change is unsafe Rust managing Vec raw-part ownership across the C++ FFI boundary, and us_socket_buffered_js_write is on the node:http server response write hot path. I traced the single in-tree caller and confirmed the take→work→update sequence is behaviorally unchanged (the new drop(self.take_stream_buffer()) at the top of update is a no-op there because the prior take already nulled the pointer). destroy now reconstructs with list_len instead of 0, which is equivalent for Vec<u8> drop. The 8-test count matches 6 new stream-buffer tests + 2 pre-existing quic/Header.rs classify tests, all FFI-free, so the Miri addition should be safe.

Other factors

No CODEOWNERS cover these paths. No prior reviewer comments to address. The bug-hunting pass found nothing. The reasoning and test coverage (including Miri mutation verification) are thorough. My only reason for not auto-approving is the criticality of the path: unsafe ownership changes at an FFI boundary in production HTTP server code warrant a human sign-off even when they look right.

robobun · 2026-06-08T10:09:02Z

CI status after the rebase (builds 112310, 112371, 112396, 112419): every hard failure is a break on main, present on every concurrent PR build in the same window.

test/js/node/test/parallel/test-crypto-dh-leak.js on linux-x64-asan: RSS grows by about 24 MB with the same numbers on all four builds and on builds 112411 through 112424 of unrelated branches.
test/js/web/timers/setInterval.test.js on linux-x64-asan: the leak test times out, also on unrelated builds 112422 and 112423.
One Windows lane on 112396 never got an agent (Azure VM creation timed out).

Both test failures are reported to main-break triage. The lanes that exercise this diff are green: cargo miri test (with bun_uws_sys), clippy, the source-lints job, all platform builds, and the node:http suites on the asan lane. Binary size unchanged.

robobun · 2026-09-04T12:09:01Z

Stale PR review: keep open, rework.

The fix for #31971 is still wanted. On main, the safe pub fn to_stream_buffer(&self) (src/uws_sys/us_socket_t.rs:639) rebuilds an owning Vec with Vec::from_raw_parts and leaves the raw parts set. A one-shot take_stream_buffer(&mut self) is the shape that REVIEW.md:22 asks for: "Neutralize the source handle when ownership transfers." No JavaScript program reaches the double free today, so this is a soundness fix with no change that users can see.

The current diff cannot merge as it is:

It conflicts with main. .github/workflows/miri.yml no longer exists. Remove dead code: 92 trait impls nothing calls, across 29 crates #39795 removed impl Default for us_socket_stream_buffer_t, which the new Rust tests call.
test/internal/uws-stream-buffer-ownership.test.ts reads the source text and checks that to_stream_buffer is gone. REVIEW.md:56 and test/internal/source-lints/CLAUDE.md say not to add tests for dead symbols.

The wanted shape:

Rebase onto main. Keep take_stream_buffer(&mut self), the change to update(), and the caller change at src/runtime/socket/uws_jsc.rs:186.
Keep the six #[cfg(test)] tests. Build the struct with a struct literal in the test module, and do not restore impl Default.
Keep the bun_uws_sys entry in MIRI_CRATES (scripts/rust-miri.ts). CI runs those tests under Miri only through that list.
Delete the source-text test.

Two open PRs touch the same line. #35018 also says that it fixes #31971, because it removes to_stream_buffer and update. #40236 keeps to_stream_buffer and changes the caller to buffer.with_mut(|b| b.to_stream_buffer()). If a maintainer merges #35018 first, close this PR as superseded.

us_socket_stream_buffer_t::to_stream_buffer took &self and rebuilt an owning Vec<u8> from raw parts without clearing them, so safe Rust could mint two owners of the same allocation (double free). Replace it with take_stream_buffer(&mut self) which nulls the raw parts, route destroy through it so teardown is idempotent, and make update drop any still-owned buffer before overwriting the parts. Add crate unit tests and run bun_uws_sys under the cargo-miri CI lane. Fixes #31971

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

…ints The lint reads only src/uws_sys/us_socket_t.rs and never runs the built binary, so it belongs in test/internal/source-lints/ where the GitHub Actions lane runs it against a released bun. Its header now cites the miri job in rust-lints.yml instead of the removed miri.yml.

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

claude

Code review found no issues

No high-confidence issues detected in this change.

robobun · 2026-09-08T01:28:24Z

Rebased onto main (ae3c3ad) and resolved three conflicts. Details are in the Notes block of the PR description. Also addressed the review comments: the comments in the diff are one line each, the source lint moved to test/internal/source-lints/ with a corrected header, and the dead-name assertion is gone.

claude

Code review found no issues

No high-confidence issues detected in this change.

robobun mentioned this pull request Jun 8, 2026

[Unsoundness] us_socket_stream_buffer_t::to_stream_buffer can duplicate raw Vec ownership #31971

Open

github-actions Bot added the claude label Jun 8, 2026

claude Bot reviewed Jun 8, 2026

View reviewed changes

robobun force-pushed the farm/42b32e6a/uws-stream-buffer-take branch from 301c914 to 217797a Compare September 8, 2026 00:26